Skip to content

Add logic to Ros2ControlManager to match ros2_control (backport #3332) #3342

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Mar 20, 2025
Merged

Add logic to Ros2ControlManager to match ros2_control (backport #3332) #3342

merged 7 commits into from
Mar 20, 2025

Conversation

mergify[bot]
Copy link

@mergify mergify bot commented Feb 12, 2025

Description

The logic for the chained controllers still does not match the expectation of ROS2 control. This PR makes a few changes:

  1. Activation/deactivation is expected to be disjoint. For example, if controller B is a dependency of A (A chains to B) but controller B is also a dependency of C (B chains to B), then the switch from A->B to C->B would cause B to be in both the activation and deactivate list. This causes ROS2 control to through an error and reject the switch. The simplifyControllerActivationDeactivation function adds the logic needed to avoid this from happening.
  2. The valid transitions for chained controllers depend asymmetrically on whether the controller is being started or stopped. If A chained to B, then when A is started, B should be automatically started. B is allowed to be started and stopped on its own if A is not running. However, if both A and B are running, B cannot be shut down alone, both must be shut down together or A alone.
  3. Lastly, ROS 2 control should not be told to shut down controllers that are not running or start controllers that are already running. Doing so can cause an error. A check is added to only start controllers that are not already running.
This is an automatic backport of pull request #3332 done by [Mergify](https://mergify.com).

@pac48 @mergify
Add logic to Ros2ControlManager to match ros2_control (#3332)
ff3427b 
* Add logic to Ros2ControlManager to match ros2_control

Signed-off-by: Paul Gesel <[email protected]>

* Add Ros2ControlManager test

Signed-off-by: Paul Gesel <[email protected]>

* move simplifyControllerActivationDeactivation to function and add doxygen

Signed-off-by: Paul Gesel <[email protected]>

* move queue.pop_back up

Signed-off-by: Paul Gesel <[email protected]>

* Update moveit_plugins/moveit_ros_control_interface/src/controller_manager_plugin.cpp

Co-authored-by: Sebastian Castro <[email protected]>

* Update moveit_plugins/moveit_ros_control_interface/src/controller_manager_plugin.cpp

Co-authored-by: Sebastian Castro <[email protected]>

* Update moveit_plugins/moveit_ros_control_interface/src/controller_manager_plugin.cpp

Co-authored-by: Sebastian Castro <[email protected]>

* pr feedback

Signed-off-by: Paul Gesel <[email protected]>

* clang fixes

Signed-off-by: Paul Gesel <[email protected]>

---------

Signed-off-by: Paul Gesel <[email protected]>
Co-authored-by: Sebastian Castro <[email protected]>
Co-authored-by: Sebastian Jahr <[email protected]>
(cherry picked from commit dbf07b1)

# Conflicts:
#	moveit_plugins/moveit_ros_control_interface/CMakeLists.txt
#	moveit_plugins/moveit_ros_control_interface/src/controller_manager_plugin.cpp
@mergify mergify bot added the conflicts label Feb 12, 2025
@mergify Mergify
Copy link
Author

mergify bot commented Feb 12, 2025

Cherry-pick of dbf07b1 has failed:

On branch mergify/bp/humble/pr-3332
Your branch is up to date with 'origin/humble'.

You are currently cherry-picking commit dbf07b1ac.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	new file:   moveit_plugins/moveit_ros_control_interface/test/CMakeLists.txt
	new file:   moveit_plugins/moveit_ros_control_interface/test/test_controller_manager_plugin.cpp

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   moveit_plugins/moveit_ros_control_interface/CMakeLists.txt
	both modified:   moveit_plugins/moveit_ros_control_interface/src/controller_manager_plugin.cpp

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

@sea-bass
Copy link
Contributor

@pac48 want to either resolve these or just not backport this to humble? Up to you.

@sea-bass sea-bass added the persistent Allows issues to remain open without automatic stalling and closing. label Mar 19, 2025
@codecov-commenter
Copy link

codecov-commenter commented Mar 19, 2025 •
edited
Loading

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 50.94%. Comparing base (954a027) to head (fb15464).
Report is 1 commits behind head on humble.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files 
@@            Coverage Diff             @@
##           humble    #3342      +/-   ##
==========================================
- Coverage   51.39%   50.94%   -0.45%     
==========================================
  Files         382      390       +8     
  Lines       31902    32513     +611     
==========================================
+ Hits        16393    16560     +167     
- Misses      15509    15953     +444

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

sea-bass
sea-bass approved these changes Mar 19, 2025
View reviewed changes
Copy link
Contributor

@sea-bass sea-bass left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @pac48 -- found another conflict in docstrings and resolved it

@sea-bass sea-bass removed conflicts persistent Allows issues to remain open without automatic stalling and closing. labels Mar 20, 2025
@sea-bass
Copy link
Contributor

Artifact upload failure to be addressed in #3388

@sea-bass sea-bass merged commit e0a73dd into humble Mar 20, 2025
7 checks passed
@sea-bass sea-bass deleted the mergify/bp/humble/pr-3332 branch March 20, 2025 01:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pac48 pac48 pac48 left review comments

@sea-bass sea-bass sea-bass approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sea-bass @codecov-commenter @pac48